Restrict `Protected` to some heap types. #6471

cheme · 2020-06-22T16:21:18Z

This PR restrict Protected types to some heap types.
The goal is to avoid wrongly using Protected over some structure on stack.
I think this should be enough for current usage, going further will probably lead to write https://github.com/iqlusioninc/crates/tree/develop/secrecy .

bkchr · 2020-06-22T17:49:30Z

primitives/core/src/crypto.rs

-impl<T: Zeroize> From<T> for Protected<T> {
-	fn from(t: T) -> Self {
+#[cfg(feature = "std")]
+impl From<String> for Protected<String> {


Can we not just open this From<T: Zeroize>?

The problem is with type as [u8; N] where zeroize is implemented but for whom Protected would be moving/copying stack memory all around. Typically String and Vec are relatively safe to use (as long as we don't reallocate), otherwhise Box would be safe but the api change a bit for it.
Then it does not change a thing with existing code, it just prevents some misuses.

Protected does not implement Copy.

Next point is that you can not implement Drop for Copyable types: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=ffa7796c63f95857904cdce1e0a3ebc5

I was more refering to moves or memory copy of the stack that result from compiling. But the PR just move this case out to avoid such discussion on the public api.
Another way to handle this could be to add a comment.

tomusdrw

Looks okay, but really requires a bit of explanations in comments on why we restrict the From implementation.

Why not do Protected<T>(Box<T>) instead? So that we are not restricted to heap-allocated types?

cheme · 2020-06-23T07:27:30Z

I had a first implementation with Box but I remember some slight api difference (at least for the sized array case) and did add a new structure for the Box case. Actually looking at 'secrecy' crate from the author of zeroize, that is exactly the three cases that get covered (box, vec and string).
Knowing myself I probably looked at the only usage being Protected<Box> and felt like it was not ok (on a different branch I simply switch Protected to a struct ProtectedString but it was way bigger diff overall).
So at this point, I did drop the box code, I think we are using this 'simple' Protected, Protected is identic code, but Protected<Box> would be adding new code, so if the need arise for it we should ask if we should use Secrecy crate (Protected for the current use looks lighter than a new dependency).
Will be adding some doc.

burdges · 2020-06-23T08:20:27Z

I think the secrecy crate serves mostly as reminder and grep fodder for whenever you access a secret. Use it directly maybe?

I'd wait for rust-lang/rfcs#2859 myself before doing anything else.

I think heap vs stack remains unconvincing without further effort, ala mlock, which all sounds like a distraction now. We've an Arc in the key store which helps avoid keys being cloned much, but obviously a Box does nothing against clone, and Vec even less.

cheme · 2020-06-23T08:35:44Z

Use it directly maybe?

I consider that, honestly it will not be to much work to replace Protected by Secrecy, but I step back because I was not sure that having to follow a new crate code was worth it. cc @tomusdrw @bkchr what do you think?

tomusdrw · 2020-06-23T09:56:17Z

I'm fine to use secrecy, the crate is pretty small (you can check the code in like 5 mins). AFAICT the point was that due to rust move semantics, stack-allocated secrets will remain in memory (drop is not being called when you move the value, but the value actually is copied), hence we want to make sure that whatever is Protected is heap-allocated, so that when Protected value is moved around only the memory pointer is being copied. And secrecy doesn't really have anything that supports this pattern, so we would need to wrap it or document correctly to make sure it's used correctly.

burdges · 2020-06-23T10:44:46Z

Yes, I suppose more discipline in how substrate handles secrets makes sense.

All rust crypto crates use almost exclusively stack for secrets. I doubt any would change this until rust-lang/rfcs#2859 and rust-lang/rfcs#2884 works, and maybe someone needs to figure out mlock from rust too, so optimistically a couple years away. It's rather a different problem in substrate though, so sure..

cheme · 2020-06-23T10:53:04Z

Last commit simply replace Protected by SecrecyString.

PoignardAzur · 2020-06-24T11:51:31Z

I'd wait for rust-lang/rfcs#2859 myself before doing anything else.

RFC co-author here.

I'm probably not saying anything you don't already know, but I just want to point out that the RFC is dead in the water so far. The lang team hasn't shown any interest yet (that might change with the new MCP process, but I'm skeptical), and until they do there isn't even a guarantee that the RFC will be officialized as something we want in Rust.

So "a couple years away" is indeed optimistic.

cecton

I think this is great! But maybe it's best to wait for other reviewers

bkchr · 2020-06-24T15:52:21Z

client/cli/src/params/keystore_params.rs

+impl Drop for KeystoreParams {
+	fn drop(&mut self) {
+		use sp_core::crypto::Zeroize;
+		self.password.zeroize();


Can we not replace password: Option<StringSecret> to remove this custom implementation of Drop here?

It implements FromStr, but structopt complain about to_string not being implemented. Could use a type wrapper, but I am not sure it is worth it.

ok, it is actually the from str error that needed to be display, so no pb.

bkchr · 2020-06-24T15:53:17Z

primitives/core/src/crypto.rs

 #[doc(hidden)]
 pub use sp_std::ops::Deref;
 use sp_runtime_interface::pass_by::PassByInner;
+#[doc(hidden)]


Why are both of these exported hidden?

cheme · 2020-06-30T17:19:04Z

@bkchr I did remove the drop on the struct opt parameter, looks better now.

bkchr

Please rename and move the secret string parse function. Otherwise it looks good.

bkchr · 2020-07-01T07:47:38Z

primitives/core/src/crypto.rs

+#[cfg(feature = "std")]
+/// Parse a sercret string, returning a displayable error.
+pub fn secrety_string_from_str(s: &str) -> Result<SecretString, String> {
+	Ok(std::str::FromStr::from_str(s)
+		.map_err(|_e| "Could not get SecretString".to_string())?)
+}


Suggested change

#[cfg(feature = "std")]

/// Parse a sercret string, returning a displayable error.

pub fn secrety_string_from_str(s: &str) -> Result<SecretString, String> {

Ok(std::str::FromStr::from_str(s)

.map_err(|_e| "Could not get SecretString".to_string())?)

}

/// Parse a sercret string, returning a displayable error.

#[cfg(feature = "std")]

pub fn secret_string_from_str(s: &str) -> Result<SecretString, String> {

std::str::FromStr::from_str(s).map_err(|_e| "Could not get SecretString".to_string())

}

And please move this to cli. As we only need this in the cli. Please put it into client/cli/src/params/mod.rs.

bkchr · 2020-07-01T09:06:47Z

bot merge

ghost · 2020-07-01T09:06:50Z

Waiting for commit status...

ghost · 2020-07-01T09:10:24Z

Head SHA has changed since merge was requested; aborting merge.

ghost · 2020-07-01T09:10:25Z

Head SHA has changed since merge was requested; aborting merge.

ghost · 2020-07-01T09:10:25Z

Head SHA has changed since merge was requested; aborting merge.

bkchr · 2020-07-01T09:12:02Z

bot merge

ghost · 2020-07-01T09:12:07Z

Waiting for commit status...

gavofyork · 2020-07-01T09:27:53Z

@cheme will need a master-merge in order to fix cargo-deny error.

ghost · 2020-07-01T09:29:33Z

Head SHA has changed since merge was requested; aborting merge.

cheme · 2020-07-01T09:31:25Z

bot merge

ghost · 2020-07-01T09:31:27Z

Waiting for commit status...

Restrict Protected to some heap types.

43f7295

cheme added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. B2-breaksapi labels Jun 22, 2020

cheme requested a review from tomusdrw June 22, 2020 16:21

cheme requested a review from NikVolf as a code owner June 22, 2020 16:21

bkchr reviewed Jun 22, 2020

View reviewed changes

tomusdrw reviewed Jun 23, 2020

View reviewed changes

tomusdrw added A5-grumble and removed A0-please_review Pull request needs code review. labels Jun 23, 2020

Comment abut Protected usage.

c7d7b07

Remove Protected from crypto, use secrecy crate for existing uses.

3f0b9b2

cheme requested a review from cecton as a code owner June 23, 2020 10:51

cheme added B3-apinoteworthy and removed B2-breaksapi labels Jun 23, 2020

cecton approved these changes Jun 24, 2020

View reviewed changes

bkchr reviewed Jun 24, 2020

View reviewed changes

cheme added 4 commits June 24, 2020 23:10

use a parse function

2438b4c

Merge branch 'master' into avoid_stack_protected

91cb0f3

Merge branch 'master' into avoid_stack_protected

56c965a

fix error convert

cf77ff3

bkchr approved these changes Jul 1, 2020

View reviewed changes

Rename and move secretY string function.

005dfc4

gavofyork added A8-mergeoncegreen and removed A5-grumble labels Jul 1, 2020

gavofyork requested a review from tomusdrw July 1, 2020 08:48

gavofyork closed this Jul 1, 2020

gavofyork reopened this Jul 1, 2020

gavofyork added A5-grumble and removed A8-mergeoncegreen labels Jul 1, 2020

std result

27cbd6a

Merge branch 'master' into avoid_stack_protected

0eeebc9

ghost merged commit 8ef1ac0 into paritytech:master Jul 1, 2020

This pull request was closed.

Restrict Protected to some heap types. #6471

Restrict Protected to some heap types. #6471

Uh oh!

Conversation

cheme commented Jun 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

cheme commented Jun 23, 2020

Uh oh!

burdges commented Jun 23, 2020

Uh oh!

cheme commented Jun 23, 2020

Uh oh!

tomusdrw commented Jun 23, 2020

Uh oh!

burdges commented Jun 23, 2020

Uh oh!

cheme commented Jun 23, 2020

Uh oh!

PoignardAzur commented Jun 24, 2020

Uh oh!

cecton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cheme commented Jun 30, 2020

Uh oh!

bkchr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchr commented Jul 1, 2020

Uh oh!

ghost commented Jul 1, 2020

Uh oh!

ghost commented Jul 1, 2020

Uh oh!

ghost commented Jul 1, 2020

Uh oh!

ghost commented Jul 1, 2020

Uh oh!

bkchr commented Jul 1, 2020

Uh oh!

ghost commented Jul 1, 2020

Uh oh!

gavofyork commented Jul 1, 2020

Uh oh!

ghost commented Jul 1, 2020

Uh oh!

cheme commented Jul 1, 2020

Uh oh!

ghost commented Jul 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Restrict `Protected` to some heap types. #6471

Restrict `Protected` to some heap types. #6471